-
Notifications
You must be signed in to change notification settings - Fork 135
chore: change streamingmode to exact for ranges with limits #3476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 11-17-fix_gas_optimize_loops
Are you sure you want to change the base?
chore: change streamingmode to exact for ranges with limits #3476
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code Review - PR #3476SummaryThis PR correctly changes Positive FindingsCorrectness ✅
Performance ✅
Consistency ✅
Code Quality ✅
Potential ConsiderationsTesting
While the semantic change is minimal (both modes should return the same results when a limit is specified), the different batching behavior could theoretically expose edge cases. Documentation
VerdictLGTM - This is a well-targeted performance optimization with no apparent bugs or issues. The change correctly applies The only suggestion is to ensure adequate test coverage for the affected code paths, though this appears to be a low-risk refactoring. |
e5b2d7f to
eda671f
Compare
539e5ed to
cd862c4
Compare
PR Review: Change StreamingMode to Exact for ranges with limitsSummaryThis PR correctly updates FoundationDB range queries to use Code Quality ✅Correctness: The changes are semantically correct. According to FoundationDB documentation:
All modified queries have explicit limits set, making Affected Files:
Performance Considerations ⚡Positive Impact: Using
Potential Issues 🔍Minor Note - Field Reordering: In // Before
begin: begin_key,
end: end_key,
mode: StreamingMode::WantAll,
limit: Some(count as usize),
// After
mode: StreamingMode::Exact,
limit: Some(count as usize),
end: end_key,
begin: begin_key,While this works fine (fields use named syntax), it's inconsistent with other files in the PR. Consider keeping the original field order for consistency, though this is purely stylistic. Missing Coverage 🔎I noticed a few files still using
These are intentionally not changed and should remain Test Coverage 📝Recommendation: While the changes are straightforward, it would be beneficial to verify that:
You could run the affected package tests: cargo test --package epoxy --package gasoline --package namespace --package pegboardSecurity Concerns 🔒No security issues identified. Verdict ✅Approved - This is a good optimization that aligns query patterns with FoundationDB best practices. The changes are minimal, focused, and semantically correct. Optional: Consider keeping consistent field ordering in Great work on identifying and fixing this pattern across the codebase! |
Code ReviewSummaryThis PR changes StreamingMode::WantAll to StreamingMode::Exact for all range queries that have explicit limit parameters. This is the correct optimization based on the FoundationDB documentation. Positives1. Correct Use of StreamingMode::Exact All modified queries have explicit limits, making Exact the appropriate mode:
2. Performance Improvement 3. Consistency Minor Observations1. Field Reordering in setup.rs 2. Test Coverage
Security and Correctness
VerdictApproved This is a clean optimization that correctly uses FoundationDB's Exact streaming mode for queries with known limits. The change improves performance without affecting correctness. Review generated with Claude Code |
cd862c4 to
fba43a9
Compare
eda671f to
2f81c6e
Compare
Code ReviewSummaryThis PR correctly changes AnalysisWhat's the difference?
When a query specifies Findings✅ Correct Changes (6 files):
tx.get_ranges_keyvalues(
universaldb::RangeOption {
mode: StreamingMode::WantAll, // <- Should be Exact
limit: Some(1),
..(&pending_signal_subspace).into()
},
// ...
)Recommendations
Performance & Correctness✅ Performance: Using ✅ Correctness: The change is semantically correct and aligns with FoundationDB best practices. ✅ No Breaking Changes: This is purely an optimization/correctness fix with no API changes. Overall AssessmentThis is a good cleanup PR that improves query efficiency. Once the missed instance is addressed, this will be ready to merge. Action Items:
|
Code ReviewSummaryThis PR changes StreamingMode::WantAll to StreamingMode::Exact for FoundationDB range queries that have explicit limits. This is a good performance optimization. Positive Observations✅ Correct optimization: According to the FoundationDB documentation, when you have a specific row limit, StreamingMode::Exact is the appropriate mode to use. The comment in the options enum states that Exact mode is for when "The client has passed a specific row limit and wants that many rows delivered in a single batch" and "A row limit must be specified if this mode is used." Using Exact with explicit limits is more efficient than WantAll because:
✅ Comprehensive coverage: The PR correctly identifies and updates all instances where mode: StreamingMode::WantAll is used with an explicit limit: Some(...). I verified that the remaining WantAll usages in the codebase do not have explicit limits, which is correct. ✅ Consistent pattern: All 6 changed files follow the same pattern - queries with known limits that should use Exact mode. Minor Observations📝 Code organization (epoxy/workflows/replica/setup.rs:446): The field order was changed, moving mode and limit before end and begin. While this doesn't affect functionality, consider maintaining consistent field ordering across similar code blocks for readability. Most other changes kept the original order. Testing RecommendationsWhile this is a performance optimization that shouldn't change behavior, consider:
VerdictApproved - This is a solid optimization that correctly applies FoundationDB best practices. The changes are straightforward and well-targeted. |
2f81c6e to
b8b2cef
Compare
fba43a9 to
00a1885
Compare
Code ReviewSummaryThis PR changes the StreamingMode from WantAll to Exact for range queries that have explicit limits. This is a correct and important optimization based on FoundationDB's documented behavior. AnalysisWhat Changed: Why This Is Correct:
When you have a known, specific limit (like limit: Some(input.limit) or limit: Some(1)), using Exact tells FoundationDB to:
Performance Impact✅ Positive: This change should improve performance by:
Code Quality✅ Consistent Pattern: All changed locations follow the same pattern:
Files Changed
Potential ConsiderationsMinor Note - Field Ordering (epoxy/src/workflows/replica/setup.rs) Test Coverage
Security & Safety✅ No security concerns identified. This is a performance optimization that doesn't change the logical behavior of the queries. VerdictLGTM with minor note ✅ This is a good optimization that properly uses FoundationDB's streaming modes. The only minor point is the field reordering in one file, but that's a very minor stylistic issue. Reviewed following the repository's CLAUDE.md conventions |

No description provided.